-
Notifications
You must be signed in to change notification settings - Fork 28.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-10479] [SPARK-10480] [ML] Fix ML.LinearRegressionModel.copy() #8641
Conversation
Looks OK. On the borderline of needing a JIRA. really this is a fix for SPARK-8538 / SPARK-8539 I think. CC @feynmanliang |
Test build #42093 has finished for PR 8641 at commit
|
LGTM, just noticed that LogisticRegression doesn't copy the summary object, SPARK-10479 will track that work |
@yanboliang how about fixing SPARK-10479 here too? it's very similar. |
@srowen OK, I have updated this PR to fix both SPARK-10479 and SPARK-10480 |
@@ -468,7 +468,9 @@ class LogisticRegressionModel private[ml] ( | |||
} | |||
|
|||
override def copy(extra: ParamMap): LogisticRegressionModel = { | |||
copyValues(new LogisticRegressionModel(uid, weights, intercept), extra).setParent(parent) | |||
val newModel = copyValues(new LogisticRegressionModel(uid, weights, intercept), extra) | |||
if (trainingSummary.isDefined) newModel.setSummary(trainingSummary.get) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, was this intended as well? does it need to happen for LinearRegression
too? LGTM otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's needed. Because after we new LogisticRegressionModel
or LinearRegressionModel
, the default value of trainingSummary
is None
. So we should also copy trainingSummary
explicitly to the new model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. newModel.trainingSummary = trainingSummary
should work the same.
Test build #42130 has finished for PR 8641 at commit
|
Please add [SPARK-10479] to PR title |
LGTM |
Merged into master and branch-1.5. Thanks! |
This PR fix two model
copy()
related issues:SPARK-10480
ML.LinearRegressionModel.copy()
ignored argumentextra
, it will not take effect when users setting this parameter.SPARK-10479
ML.LogisticRegressionModel.copy()
should copy model summary if available.